-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add configuration for controlling the autonaming behavior #1831
Conversation
This PR adds some new functionality to control the auto naming behavior. The new behavior lives behind a provider config variable and must be explicitly enabled by the user. The existing behavior will remain the default behavior of the provider. **What's new** - `autoTrim`: When this is set to true the provider will automatically trim the generated name to fit within the `maxLength` requirement. - `randomSuffixMinLength`: Set this to control the minimum length of the random suffix that is generated. This is especially useful in combination with `autoTrim` to ensure that you still end up with unique names (e.g. a random suffix of 1 character is not very unique) closes #1816, re pulumi/pulumi-cdk#62
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1831 +/- ##
==========================================
+ Coverage 49.62% 49.96% +0.34%
==========================================
Files 43 43
Lines 6591 6636 +45
==========================================
+ Hits 3271 3316 +45
Misses 3077 3077
Partials 243 243 ☔ View full report in Codecov by Sentry. |
if left <= 0 && autoTrim { | ||
autoTrimMaxLength := autoNamingSpec.MaxLength - namingTrivia.Length() - randomSuffixMinLength | ||
if autoTrimMaxLength <= 0 { | ||
return resource.PropertyValue{}, fmt.Errorf("failed to auto-generate value for %[1]q."+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So surprised to see Fortran-style 1-based indices here but I think this checks out. This is right.
autoNameConfig: AutoNamingConfig{ | ||
AutoTrim: true, | ||
}, | ||
comparison: within(12, 12), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we can see equals(resource.NewStringProperty(".."))
here or it's omitted because it is random?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I might be able to do a regex check because only the end should be random.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Added #1833 but out of scope of this PR.... Looks like we are consuming GetVariables still in this provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
Assuming here pulumi/pulumi#17592 is not quite ready for prime time so we need this now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -101,3 +135,25 @@ func getDefaultName( | |||
|
|||
return resource.NewStringProperty(random), nil | |||
} | |||
|
|||
// trimName will trim the prefix to fit within the max length constraint. | |||
// It will cut out part of the middle, keeping the beginning and the end of the string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the right thing to do given the current constraints. But it could also be rather surprising to users.
For example, I often split my resource names with hyphens. This could generate some rather odd names.
But given that pulumi/pulumi#17592 is not ready yet, I think this is a good choice.
If we see the need for it, we could also expose more configuration options for autonaming in the future.
@@ -519,6 +520,14 @@ func (p *cfnProvider) Configure(ctx context.Context, req *pulumirpc.ConfigureReq | |||
metadata.CfnCustomResourceToken: resources.NewCfnCustomResource(p.name, s3Client, lambdaClient), | |||
} | |||
|
|||
if autoNaming, ok := vars["aws-native:config:autoNaming"]; ok { | |||
var autoNamingConfig autonaming.AutoNamingConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test the unmarshalling? It seems to be untested right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
Adding `startsWith` comparison to better assert autoTrim
This PR adds some new functionality to control the auto naming behavior.
The new behavior lives behind a provider config variable and must be
explicitly enabled by the user. The existing behavior will remain the
default behavior of the provider.
What's new
autoTrim
: When this is set to true the provider will automaticallytrim the generated name to fit within the
maxLength
requirement.randomSuffixMinLength
: Set this to control the minimum length of therandom suffix that is generated. This is especially useful in
combination with
autoTrim
to ensure that you still end up withunique names (e.g. a random suffix of 1 character is not very unique)
closes #1816, re pulumi/pulumi-cdk#62